fix(sync): retry /save when the app does not answer, and carry feeds.opml - #380
Merged
Conversation
…opml Two changes to settings sync. **Retry on 5xx.** `request()` was a single fetch: any non-2xx became a value the caller printed before exiting, so one transient 502 was a visible failure. app.moshcode.sh returns "Application failed to respond" intermittently - a probe of the write path measured 20/20 healthy minutes after a real save had failed with exactly that - and unretried, every blip is a failed save. It felt frequent because every one of them was. Retried only where a retry can be right. 502, 503, 504 and a dead socket say nothing about the request: the platform returned them without the app seeing it, so the same bytes a second later are as likely to work as they were the first time. Every other status is an answer - 400 will be 400 again, 401 wants /login, 409 is another machine having saved first and retrying would only lose the same race again. Safe to repeat a PUT because the write is conditional: `ifRevision` pins the revision the caller last agreed on, so a retry landing after a first attempt secretly succeeded is refused with 409 rather than writing twice. Two retries at 400ms and 1200ms, then it is news. The scan workflow this repository ships already retries `npm install` three times for precisely this reasoning; the sync client had none. **feeds.opml joins SYNCED_FILES.** A feed list is exactly the sort of thing that should follow a person to their next machine, and OPML is the format every reader already imports and exports. `json: false`: moshcode's interest in the file begins and ends with moving it, so unlike aliases.json there is no such thing as a broken one here. The server needs nothing. It validates shape rather than filenames - no `..`, no leading slash, 32 files, 256KB - so it already accepts this. Five tests: OPML travels byte for byte, a non-XML feed list still syncs, a 502 is ridden out without a line about it, a 409 is asked exactly once, and a 502 that never clears is still reported rather than retried forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan97 finding(s) HIGH/CRITICAL: 5 | MEDIUM: 41 | LOW: 51
…and 47 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
added a commit
that referenced
this pull request
Aug 13, 2026
…385) `/save` failed with 502 every time unless you passed --force. The cause is one missing clause, and it survived because it is invisible to the test suite. insertRevision() builds two statements. The --force path sends ifRevision: null and inserts unconditionally. The ordinary path carries a precondition and expressed it as a HAVING on an implicit single-group aggregate: INSERT INTO settings_snapshots (…) SELECT … FROM settings_snapshots WHERE user_id = ? HAVING COALESCE(MAX(revision),0) = ? SQLite treats the whole result as one group and runs that happily. Turso's parser rejects it outright: SQL string could not be parsed: near HAVING, "None": syntax error The route threw, the platform returned 502, and the CLI reported exactly what it saw. #380 added retries for "the app does not answer", which could never help: the statement is deterministically unparseable, so every attempt failed the same way. The tests could not catch it. They run against `file:` — a different engine from the deployment — and test/settings-sync.mjs does cover the precondition, with a stale ifRevision refused as 409 and a current one accepted. It passed throughout, because on SQLite the statement is valid. Adding GROUP BY user_id makes it parse on both. It cannot change the answer: the caller sets ifRevision to null when the account has no current revision, so there is always at least one row for this user to group. Verified against the real database rather than only locally — the bare form fails to parse on Turso, the grouped form parses and inserts nothing when the precondition does not hold, which is the conflict the caller reports as 409. test/sql-portability.test.mjs now fails on any HAVING in src/ with no GROUP BY. Static rather than behavioural on purpose: the behaviour is correct on the engine the tests use, so only reading the SQL can catch this. It matches the uppercase keyword with comments stripped, because "having" is also an English word and four files that contain no SQL said it in prose. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes to settings sync, from chasing a
/savethat kept failing./savehad no retry at allrequest()was a singlefetch: any non-2xx became a value the caller printed before exiting. So one transient blip was a visible failure — which is why it felt frequent. Every one of them was.The failure is real and intermittent:
Minutes later the identical payload returned 200, and a 20-probe sweep of the write path came back 20/20 healthy. The app is up; it just occasionally does not answer.
Retried only where a retry can be right
/login, and says soTwo retries at 400ms and 1200ms, then it is news.
Repeating a PUT is safe because the write is conditional.
ifRevisionpins the revision the caller last agreed on, so a retry landing after a first attempt secretly succeeded is refused with 409 rather than writing twice.Worth noting the asymmetry this fixes: the ThreatCrush scan workflow this repository ships retries
npm installthree times, with a comment explaining that an unretried network call decides whether a security gate runs at all. The sync client had none.feeds.opmljoinsSYNCED_FILESA feed list is exactly the sort of thing that should follow a person to their next machine, and OPML is the format every reader already imports and exports. Written by
tcfeed rss add(profullstack/threatcrush#118) and read by nothing here.json: falseon purpose — moshcode's interest in the file begins and ends with moving it. Unlikealiases.json, where a broken file is held back rather than copied to every machine you own, there is no such thing as a broken one here.The server needs nothing.
snapshotProblem()validates shape rather than filenames — no.., no leading slash, ≤32 files, ≤256KB — so it already accepts this. No deploy.Tests
node --test test/settings-sync.test.mjs→ 32 pass, 0 fail (27 before). Five new:The existing "a network failure is a line and an exit code, never a throw" now takes 1.6s rather than 0 — that is the new backoff being exercised by a test that already existed.
🤖 Generated with Claude Code